Skip to content

Comments

[Access] Properly handle subscription errors in data providers#7046

Merged
peterargue merged 22 commits intomasterfrom
illia-malachyn/7040-fix-context-canceled-error-propagation
Mar 21, 2025
Merged

[Access] Properly handle subscription errors in data providers#7046
peterargue merged 22 commits intomasterfrom
illia-malachyn/7040-fix-context-canceled-error-propagation

Conversation

@illia-malachyn
Copy link
Contributor

@illia-malachyn illia-malachyn commented Feb 17, 2025 •
edited
Loading

Originally, the description of the issue was like that

Distinguish between `context.Canceled` errors originating from the streamer and those triggered by the DataProvider’s `Close()` method.

1. I Added `closeChan` that is used in `DataProvider.Close()` method to indicate `DataProvider.Run()` that users of data providers (WebSocket controller in our case) want to finish receiving data. 
2. `HandleSubscription()` function is replaced by `run()` function that is aware of `closeChan`. I made a new function for it because `HandleSubscription()` is widely used in the access package (`HandleRPCSubscription` has 22 usages atm).
3. `run()` returns nil if `closeChan` was closed. `HandleSubscription` returned `ctx.Canceled` which lead to confusion as `ctx.Canceled` could come from 2 sources (streamer and websocket controller).
4. I did a little refactoring to a bunch of `sendResponse()` functions to make it more readable.

However, after a couple of discussions and work on that, I can conclude that the first 3 clauses are outdated and were removed from the implementation. The only thing we left is the refactoring and the fact that we start the ubscription after a Run() method has been called.

Closes #7040 #7047

@illia-malachyn
Properly handle subscription errors in data providers
8f6ca1a 
Distinguish between `context.Canceled` errors originating from the
streamer and those triggered by the DataProvider’s `Close()` method.
Use `wasClosedByClient()` to suppress expected cancellations while
propagating unexpected ones
@illia-malachyn illia-malachyn requested a review from a team as a code owner February 17, 2025 14:22
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.34505% with 49 lines in your changes missing coverage. Please review.

Project coverage is 41.25%. Comparing base (82d7f1d) to head (429b961).

Files with missing lines Patch % Lines
engine/access/rest/common/models/block.go 47.27% 26 Missing and 3 partials ⚠️
...ss/rest/websockets/data_providers/base_provider.go 67.85% 6 Missing and 3 partials ⚠️
...ockets/data_providers/account_statuses_provider.go 92.68% 2 Missing and 1 partial ⚠️
.../rest/websockets/data_providers/blocks_provider.go 91.42% 2 Missing and 1 partial ⚠️
...ckets/data_providers/mock/data_provider_factory.go 40.00% 3 Missing ⚠️
.../rest/websockets/data_providers/args_validation.go 80.00% 1 Missing ⚠️
.../rest/websockets/data_providers/events_provider.go 97.36% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #7046   +/-   ##
=======================================
  Coverage   41.25%   41.25%           
=======================================
  Files        2170     2170           
  Lines      190050   190098   +48     
=======================================
+ Hits        78409    78429   +20     
- Misses     105097   105123   +26     
- Partials     6544     6546    +2
Flag Coverage Δ
unittests 41.25% <84.34%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Guitarheroua
Guitarheroua previously approved these changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@Guitarheroua Guitarheroua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@illia-malachyn
Copy link
Contributor Author

illia-malachyn commented Feb 17, 2025 •
edited
Loading

After discussing with Yurii, we agreed to use a different approach here.

@illia-malachyn illia-malachyn marked this pull request as draft February 17, 2025 15:02
@illia-malachyn illia-malachyn changed the title Properly handle subscription errors in data providers [DRAFT] [Access] Properly handle subscription errors in data providers Feb 17, 2025
@Guitarheroua Guitarheroua dismissed their stale review February 18, 2025 13:43

As this will changed, will re-aprove final implementation

@illia-malachyn
Add one shot close channel to all data providers
8230193 
We use it to distinguish place where cxt.Canceled
error comes from.

Also, I refactored each data provider's Run()
function. Now it's more readable and clear.
@illia-malachyn
do not spawn unnecessary routine in close()
24e95b8
@illia-malachyn illia-malachyn marked this pull request as ready for review February 18, 2025 18:15
@illia-malachyn illia-malachyn changed the title [DRAFT] [Access] Properly handle subscription errors in data providers [Access] Properly handle subscription errors in data providers Feb 19, 2025
peterargue
peterargue reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this refactor needed? can you point out the important changes, it's not clear to me

@illia-malachyn
Copy link
Contributor Author

illia-malachyn commented Feb 20, 2025

why is this refactor needed? can you point out the important changes, it's not clear to me

Hey. I updated PR's description. Added a context of what has been done and why.

illia-malachyn
illia-malachyn commented Feb 20, 2025
View reviewed changes
illia-malachyn
illia-malachyn commented Feb 20, 2025
View reviewed changes
@illia-malachyn
Copy link
Contributor Author

illia-malachyn commented Feb 20, 2025

@peterargue I also pointed out the most important lines of code

Guitarheroua
Guitarheroua reviewed Feb 21, 2025
View reviewed changes
Guitarheroua
Guitarheroua reviewed Feb 21, 2025
View reviewed changes
Copy link
Contributor

@Guitarheroua Guitarheroua left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first round of review - DONE! I have a few small comments.

Guitarheroua
Guitarheroua approved these changes Feb 27, 2025
View reviewed changes
Copy link
Contributor

@Guitarheroua Guitarheroua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@illia-malachyn illia-malachyn marked this pull request as draft February 27, 2025 18:31
@illia-malachyn illia-malachyn marked this pull request as ready for review February 27, 2025 18:31
peterargue
peterargue reviewed Mar 14, 2025
View reviewed changes
@illia-malachyn
Rework how data providers start serving data
41a80c5 
- Moved creation and start of subscription and streamer
to Run() function instead of having it in constructor

- Ged rid of ctx in constructor. Moved it to Run()
function

- Refactored the structure of the base provider
@illia-malachyn
fix comments
3b67bb3
peterargue
peterargue reviewed Mar 18, 2025
View reviewed changes
peterargue
peterargue reviewed Mar 19, 2025
View reviewed changes
peterargue
peterargue reviewed Mar 20, 2025
View reviewed changes
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small improvement, then this is ready to merge

@peterargue peterargue added this pull request to the merge queue Mar 21, 2025
Merged via the queue into master with commit 7faffd0 Mar 21, 2025
56 checks passed
@peterargue peterargue deleted the illia-malachyn/7040-fix-context-canceled-error-propagation branch March 21, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Guitarheroua Guitarheroua Guitarheroua approved these changes

@peterargue peterargue peterargue approved these changes

@UlyanaAndrukhiv UlyanaAndrukhiv Awaiting requested review from UlyanaAndrukhiv

@AndriiDiachuk AndriiDiachuk Awaiting requested review from AndriiDiachuk

@durkmurder durkmurder Awaiting requested review from durkmurder

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Access] Data providers should wrap context.Canceled error

4 participants

@illia-malachyn @codecov-commenter @Guitarheroua @peterargue